Skip to content
This repository has been archived by the owner on Oct 26, 2020. It is now read-only.

extend ProjectionNames to include Args #10

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

kirach
Copy link

@kirach kirach commented Nov 5, 2019

The copy of sangria-graphql#430 for this repo.

All credits goes to @robconrad. I've just moved the changes and resolved the conflicts.

From the original PR:

This PR extends the Projector facility with access to the Args present at each projection node. This enables use cases for n+1 query mitigation that require access to arguments in order to properly formulate the query.

I bumped the version by one patch increment, but this change may in fact require a minor version bump because anyone relying on case comprehension on the ProjectedName class will have their code broken if they pick up this build.

Addresses sangria-graphql#427

Copy link

@yanns yanns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.
We have to document that this is a breaking change.

@kirach
Copy link
Author

kirach commented Nov 5, 2019

@yanns thank you for approving. Do we have any process for documenting breaking changes in place?

@yanns
Copy link

yanns commented Nov 5, 2019

@kirach no there's no process.
I created the label for it so that we can think about it when writing the release notes.

@yanns yanns merged commit 9309733 into sangria-graphql-org:master Nov 5, 2019
@kirach
Copy link
Author

kirach commented Nov 5, 2019

@yanns got it, thank you! Let me know if you need help with anything else.

@robconrad
Copy link

Thanks guys, this is fantastic! I've been hoping to get this merged for almost a year.

@yanns
Copy link

yanns commented Jan 7, 2020

This PR introduces an issue in one application:

sangria.execution.AttributeCoercionError: Error during attribute coercion. Violations:
Variable 'skus' is used in a place where it is not allowed to be used.
	at sangria.schema.Args$.convert(Context.scala:392)
	at sangria.schema.Args$.$anonfun$apply$4(Context.scala:370)
	at scala.Option.flatMap(Option.scala:271)
	at sangria.schema.Args$.$anonfun$apply$3(Context.scala:370)
	at scala.collection.TraversableLike.$anonfun$map$1(TraversableLike.scala:238)
	at scala.collection.immutable.Set$Set1.foreach(Set.scala:97)
	at scala.collection.TraversableLike.map(TraversableLike.scala:238)
	at scala.collection.TraversableLike.map$(TraversableLike.scala:231)
	at scala.collection.AbstractSet.scala$collection$SetLike$$super$map(Set.scala:51)
	at scala.collection.SetLike.map(SetLike.scala:104)
	at scala.collection.SetLike.map$(SetLike.scala:104)
	at scala.collection.AbstractSet.map(Set.scala:51)
	at sangria.schema.Args$.apply(Context.scala:368)
	at sangria.schema.Args$.apply(Context.scala:382)
	at sangria.execution.Resolver$$anonfun$sangria$execution$Resolver$$loop$1$1.$anonfun$applyOrElse$1(Resolver.scala:1168)

With such a query:

query bySkus($skus: [String!]) {
  products(skus: $skus) {
    results {
      masterData {
        current {
          allVariants(skus: $skus) {
            sku
          }
        }
      }
    }
  }
}

I'll search for the root cause.

@kirach
Copy link
Author

kirach commented Jan 7, 2020

@yanns yeah I know what's the root of this issue - I fixed it the internal fork of sanria we're using at our company. I can open another PR to backport it here as well.

@yanns
Copy link

yanns commented Jan 7, 2020

@kirach yes please

@yanns
Copy link

yanns commented Jan 9, 2020

I'll revert this PR as it breaks productive applications.
@kirach Could you open a PR later that reverts the revert + adds the fix?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants